fix(sandbox): track PTY state per SSH channel to fix terminal resize#573
Open
cluster2600 wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(sandbox): track PTY state per SSH channel to fix terminal resize#573cluster2600 wants to merge 1 commit intoNVIDIA:mainfrom
cluster2600 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
Thank you for your interest in contributing to OpenShell, @cluster2600. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
|
Thank you for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text: I have read the DCO document and I hereby sign the DCO. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the DCO Assistant Lite bot. |
Contributor
Author
|
I have read the DCO document and I hereby sign the DCO. |
cluster2600
force-pushed
the
fix/543-pty-channel-tracking
branch
from
834c075 to
72892c7
Compare
window_change_request previously applied resize to a single shared pty_master field, ignoring the channel ID. When multiple channels were open, resize requests would affect the wrong terminal or be lost entirely. Replace the flat pty_master/input_sender/pty_request fields in SshHandler with a HashMap<ChannelId, ChannelState> so each channel tracks its own PTY master, input sender, and pending PTY request. This ensures window_change_request, data, and channel_eof all operate on the correct channel. Closes NVIDIA#543
cluster2600
force-pushed
the
fix/543-pty-channel-tracking
branch
from
72892c7 to
8c9f987
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #543 — terminal resize (
window_change_request) was broken when multiple SSH channels were open simultaneously.pty_master/input_sender/pty_requestfields inSshHandlerwith aHashMap<ChannelId, ChannelState>, ensuring each channel tracks its own PTY resources independentlyset_winsizeto pass a reference (&winsize) toioctl, correcting undefined behaviour on aarch64set_winsizefails, rather than silently discarding the resultRoot Cause
The previous implementation stored a single
pty_masterfile descriptor at the handler level. When a client opened multiple channels (e.g. parallel shells, shell + SFTP),window_change_requestwould resize whichever PTY was stored last — not the one belonging to the requesting channel.graph TD subgraph "Before (broken)" A[Channel 0 — resize 80×24] --> B[SshHandler.pty_master] C[Channel 1 — resize 120×50] --> B B --> D["ioctl(TIOCSWINSZ) on last-stored FD"] end subgraph "After (fixed)" E[Channel 0 — resize 80×24] --> F["channels[0].pty_master"] G[Channel 1 — resize 120×50] --> H["channels[1].pty_master"] F --> I["ioctl(TIOCSWINSZ) on channel 0 FD"] H --> J["ioctl(TIOCSWINSZ) on channel 1 FD"] endChanges
crates/openshell-sandbox/src/ssh.rsChannelStatestruct; migrate all per-channel fields intoHashMap<ChannelId, ChannelState>; updatepty_request_request,window_change_request,data,channel_eof,subsystem_request, andspawn_shell_or_execto look up channel state by IDTest Plan
cargo test -p openshell-sandbox— 319 passed (1 pre-existing failure indrop_privileges, unrelated)set_winsize_applies_to_correct_pty— verifies two PTYs can be resized independentlychannel_state_independent_input_senders— verifies data routing and EOF isolation between channelssequenceDiagram participant Client participant SshHandler participant Channel0 participant Channel1 Client->>SshHandler: pty_request(channel=0, 80×24) SshHandler->>Channel0: store PTY master + request Client->>SshHandler: pty_request(channel=1, 120×50) SshHandler->>Channel1: store PTY master + request Client->>SshHandler: window_change(channel=0, 132×43) SshHandler->>Channel0: ioctl(TIOCSWINSZ, 132×43) Note over Channel1: unaffected Client->>SshHandler: channel_eof(channel=0) SshHandler->>Channel0: drop input_sender Note over Channel1: still functional